Skip to content

Conversation

@astroshim
Copy link
Contributor

What is this PR for?

This PR fixes ZEPPELIN-1417.

What type of PR is it?

Bug Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1417

How should this be tested?

  1. click edit whatever interpreter you want on interpreter setting page.
  2. check the Connect to existing process and set host or port value.
  3. uncheck the Connect to existing process and save
  4. run paragraph with just you saved interpreter.

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@astroshim astroshim changed the title change to create local interpreter when option is false. [ZEPPELIN-1417] Bugfix of "Connect to existing process" Sep 7, 2016
@bzz
Copy link
Member

bzz commented Sep 7, 2016

Looks good to me, thank you @astroshim

I'm a bit confused - in JIRA fix-for version is only 0.6.2 but PR is to master branch (which implies 0.7.0 as well)

Could you please clarify, which releases shall this be part of?
Also, please feel free to assign issue to yourself so attribution will not get lost on next release!

@astroshim
Copy link
Contributor Author

@bzz Thank you for review and sorry for making you confused.
I'll fix version to 0.7.0.

@bzz
Copy link
Member

bzz commented Sep 7, 2016

Merging to master if there is no further discussion

for (InterpreterInfo info : interpreterInfos) {
if (option.isRemote()) {
if (option.isConnectExistingProcess()) {
if (option.isExistingProcess()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think InterpreterOption.isConnectExistingProcess() is no longer used in anywhere in the code base.
To avoid confusion between InterpreterOption.isConnectExistingProcess() and InterpreterOption. isExistingProcess(), @astroshim could you remove InterpreterOption.isConnectExistingProcess() method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Leemoonsoo You're right. I just removed. Thanks.

@Leemoonsoo
Copy link
Member

LGTM and merge if there're no more discussions

@asfgit asfgit closed this in 383402d Sep 15, 2016
pedrozatta pushed a commit to pedrozatta/zeppelin that referenced this pull request Oct 27, 2016
### What is this PR for?
This PR fixes [ZEPPELIN-1417](https://issues.apache.org/jira/browse/ZEPPELIN-1417
).

### What type of PR is it?
Bug Fix

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1417

### How should this be tested?
1. click `edit` whatever interpreter you want on interpreter setting page.
2. check the `Connect to existing process` and set `host` or `port` value.
3. uncheck the `Connect to existing process` and save
4. run paragraph with just you saved interpreter.

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: astroshim <[email protected]>

Closes apache#1411 from astroshim/ZEPPELIN-1417 and squashes the following commits:

0ff0f4e [astroshim] remove isConnectExistingProcess()
c84046c [astroshim] change to create local interpreter when option is false.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants